Fix port conflict error message and cleanup on failure#40102
Fix port conflict error message and cleanup on failure#40102beena352 wants to merge 7 commits intomicrosoft:feature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates WSLC container startup to surface a correct “port already in use” error to users (instead of a misleading “distribution already exists” message) and ensures containers are cleaned up when Start() fails during wslc container run.
Changes:
- Switch port-conflict handling to
THROW_HR_WITH_USER_ERROR(_IF)so a user-facing message can be surfaced. - Add error translation around
MapPort()to try to convert port-conflict failures into a clearer user error. - Fix
wslc container runfailure cleanup by only disabling auto-delete after a successfulStart().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/windows/wslcsession/WSLCContainer.cpp | Improves port-conflict error surfacing during port allocation/mapping for container open/start. |
| src/windows/wslc/services/ContainerService.cpp | Ensures container run doesn’t leak containers when Start() fails by keeping auto-delete enabled until after a successful start. |
| THROW_HR_WITH_USER_ERROR_IF( | ||
| HRESULT_FROM_WIN32(ERROR_ALREADY_EXISTS), | ||
| !allocation, | ||
| "Port %hu is in use, cannot open container %hs", | ||
| e.VmPort, | ||
| dockerContainer.Id.c_str()); | ||
| std::format(L"Port {} is already in use, cannot open container {}", e.VmPort, dockerContainer.Id), | ||
| !allocation); |
There was a problem hiding this comment.
The new user-facing port-conflict strings are hard-coded here. Elsewhere in the codebase, errors surfaced via THROW_HR_WITH_USER_ERROR(_IF) typically come from wsl::shared::Localization::MessageWslc* so they can be localized (e.g., this file uses MessageWslcVolumeNotFound). Consider adding a localized message helper for this port-in-use case and using it here.
| "Port %hu is in use, cannot start container %hs", | ||
| e.ContainerPort, | ||
| m_id.c_str()); | ||
| std::format(L"Port {} is already in use, cannot start container {}", e.VmMapping.HostPort(), m_id), |
There was a problem hiding this comment.
In host networking mode, this branch allocates a VM port using e.ContainerPort. If that allocation fails, the conflicting resource is the VM/container port, but the user error message currently reports e.VmMapping.HostPort() (Windows host port), which can differ from e.ContainerPort in host mode (e.g., -p 8080:80). Consider reporting the port you attempted to allocate (typically e.ContainerPort / VM port) here, and reserve HostPort() for failures coming from MapPort() (Windows bind).
| std::format(L"Port {} is already in use, cannot start container {}", e.VmMapping.HostPort(), m_id), | |
| std::format(L"Port {} is already in use, cannot start container {}", e.ContainerPort, m_id), |
…ttps://github.com/beena352/WSL into user/beenachauhan/fix-port-conflict-error-message
|
Hey @beena352 👋 — Following up on this draft PR. There are 2 unresolved review threads:
Is this change still being worked on? Addressing those review items would help move this forward. |
Summary of the Pull Request
Fix port conflict error message in wslc container run. When starting a container on a port that is already in use, users previously saw the misleading message “A distribution with the supplied name already exists.” With this fix, the correct error is shown (for example, “Port 8080 is already in use, cannot start container ”). Also clean up the container left behind when Start() fails.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed